-
Notifications
You must be signed in to change notification settings - Fork 152
composable queries #216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
composable queries #216
Conversation
|
|
@tanstack/db-example-react-todo commit: |
|
Size Change: +233 B (+0.79%) Total Size: 29.9 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 665 B ℹ️ View Unchanged
|
|
Really nice! Combining them is nice too: const usersQuery = defineQuery(q => q.from({ user: usersCollection }))
const users = useLiveQuery(usersQuery
.where(userIsAdult)
.select(userNameUpper)
)Why define vs. create? I generally prefer sticking with as few of verbs as possible and create seems fine here? E.g. |
|
As there is two steps, crating a query definition and then using that query definition to crate a live query that materialises to a collection (once it started), I wanted to give it a distinct name to indicate that. A defined query can be used in multiple I suppose it's a bit like a class, you define it once, then create instances of it with arguments... (FYI you can use I'm very open to naming suggestions on both! |
|
I've fleshed out jsdoc and tests for both methods. We just need to decide on naming. |
|
I was wondering why we need a callback based approach to define reusable queries. I went into the implementation and export function defineQuery<TQueryBuilder extends QueryBuilder<any>>(
fn: (builder: InitialQueryBuilder) => TQueryBuilder
): TQueryBuilder {
return fn(new BaseQueryBuilder())
}So the callback approach is unnecessary here as the user could directly define the query like this: new BaseQueryBuilder()
.from({ persons: collection })
.where(({ persons }) => gt(persons.age, 30))
.select(({ persons }) => ({
id: persons.id,
name: persons.name,
age: persons.age,
}))If we don't like the verbosity of q()
.from({ persons: collection })
.where(({ persons }) => gt(persons.age, 30))
.select(({ persons }) => ({
id: persons.id,
name: persons.name,
age: persons.age,
})) |
| * ) | ||
| * ``` | ||
| */ | ||
| export function defineForRow<TNamespaceRow extends NamespacedRow>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defineForRow function seems to be used only for typing purposes. I'm against introducing runtime functions for compile time concerns (like typing).
| * query.where(userIsActive) | ||
| * ``` | ||
| */ | ||
| const callback = <TResult>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function as well as select are confusing me. Both are basically identity functions. I don't see how this is useful. Seems like unnecessary wrapping.
Since they are identity functions, it seems that your example from the PR description is equivalent to:
type User = {
name: string
age: number
}
const userIsAdult = ({ user: User }) => gt(user.age, 18)
export const userNameUpper = ({ user }) => ({
name: upper(user.name),
})
const users = useLiveQuery((q) =>
q
.from({ user: usersCollection }) // usersCollection is a Collection<User>
.where(userIsAdult)
.select(userNameUpper)
)This seems more compact and simpler than the original code in the PR description.
|
I was chatting with @samwillis about this PR. I'm proposing this simpler approach: const query = (q: QueryBuilder) =>
q
.from({ persons: collection })
.where(({ persons }) => gt(persons.age, 30))
.select(({ persons }) => ({
id: persons.id,
name: persons.name,
age: persons.age,
})))
// Then use it later
const {data} = useLiveQuery(query)No need to call Regarding subqueries i would propose this: // Inside a query we get a reference
const userIsAdult = ({ user: Ref<User> }) => gt(user.age, 18)
export const userNameUpper = ({ user: Ref<User> }) => ({
name: upper(user.name),
})
const users = useLiveQuery((q) =>
q
.from({ user: usersCollection }) // usersCollection is a Collection<User>
.where(userIsAdult)
.select(userNameUpper)
)This is the simplest approach since you don't have to call any additional functions like Essentially, my proposal boils down to using Javascript's built-in abstraction mechanisms rather than introducing our own ( |
|
I was thinking the exact same thing as Kevin. |
|
Yep, we chatted it over and are going to go with the simpler option. It's then just docs and renaming a few of the internal types 👍 Closing this |
stacked on #185 - brings more composability to queries, and looking for feedback.
The first part is essentially done, a
defineQueryfunction that allows to you define a query outside of creating a live query collection. It can then be passed to eitherliveQueryCollectionOptionsor touseLiveQueryas both a rootfromor used in ajoinalong with any number of rested subqueries.The second part is very much draft, I've been looking at ways to define parts of a query for later composing. after much experimenting this is what I have, an api to help define reusable callbacks to pass to the any method that takes a callback (where/having/orderby/join etc).
It's slightly odd, in order to make the type inference work:
Note how you have to call
defineForRow<interface>()first, then a method on the returned object. This is required to make the full type inference work, mostly for the return type of the callback.(I've named it
defineForRow, but I don't like that name)